fix: use (name, subtype) tuple as component key in DeprecationDetector#498
Conversation
✅ Deploy Preview for otel-ecosystem-explorer ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jaydeluca
left a comment
jaydeluca
left a comment
There was a problem hiding this comment.
most of these changes seem unrelated to the PR
| @@ -0,0 +1,47 @@ | |||
| # V1 Registry Sync | |||
There was a problem hiding this comment.
i think this file is unrelated to this PR?
There was a problem hiding this comment.
Hi @jaydeluca, you are right, those extra commits got pulled in from an earlier branch I was working on and they have nothing to do with this fix. I have rebased the branch so it now only contains the two files that were actually changed for this issue: deprecation_detector.py and test_deprecation_detector.py. All 8 tests pass locally. Sorry for the noise, please take another look when you get a chance.
The _build_component_set method was identifying components by name alone. This caused the detector to miss removals when two components shared the same name but had different subtypes, for example two extensions named zipkin under encoding and storage subtype groups respectively. Changed the set to hold (name, subtype) tuples so each component is uniquely identified. Updated the removal check in detect_deprecated to match on the full tuple. Added two new test cases that cover subtype removal detection and confirm no false positives are produced when the same name survives under an unchanged subtype. Closes open-telemetry#493
Rama542
force-pushed
the
fix/deprecation-detector-subtype-key
branch
from
d891447 to
1c395f6
Compare
There was a problem hiding this comment.
Pull request overview
Fixes collector component deprecation detection to correctly distinguish extension components that share a name but differ by subtype, preventing missed deprecation entries for nested extension components.
Changes:
- Updated
DeprecationDetectorto use(name, subtype)tuples as the unique component key when computing removals. - Updated deprecation detection logic to compare against those tuple keys.
- Added test coverage for subtype-aware removals and to ensure no false positives when a subtype’d component remains present.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ecosystem-automation/collector-watcher/src/collector_watcher/deprecation_detector.py | Switches removed-component detection from name-only to (name, subtype) keys and updates the membership check accordingly. |
| ecosystem-automation/collector-watcher/tests/test_deprecation_detector.py | Adds regression tests covering subtype removals and ensuring subtype’d components are not incorrectly marked deprecated. |
| def test_detect_deprecated_same_name_different_subtype_no_false_positive(detector, previous_version, current_version): | ||
| """When a component exists under the same name but its subtype changes, | ||
| only the removed subtype entry should be flagged, not the surviving one.""" |
Fixes #493
What was wrong
The
_build_component_setmethod inDeprecationDetectorwas building a plain set of component names to figure out which components had been removed between versions. That works fine when every component in a list has a unique name, but collector extension components can have asubtypefield (encoding, observer, storage) because they live in nested directories. Two extension components with the same name but different subtypes are completely separate things, yet the old code treated them as the same.A concrete example: if you had a
zipkinencoding extension and azipkinstorage extension in the previous version, and then the encoding one got removed in the next version, the detector would seezipkinin both the old and new sets and conclude nothing was removed. The deprecation would go untracked.What I changed
Changed the set to hold
(name, subtype)tuples instead of plain names. Updated the check insidedetect_deprecatedto match against those tuples. The variable was also renamed fromremoved_namestoremoved_keysto reflect that it now holds tuples rather than plain strings.Tests
Added two new test cases in
test_deprecation_detector.py:test_detect_deprecated_subtype_removal: verifies that removing one subtype entry while keeping another with the same name is correctly detected as a deprecation.test_detect_deprecated_same_name_different_subtype_no_false_positive: verifies that when a component with a specific subtype survives unchanged, no false deprecation is reported.All 8 tests pass.